DataViews: Fix patterns, templates and template parts pagination z-index
#58965
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Bug
ntsekouras
commented
Feb 13, 2024
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +51 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
The same issue exists on the Templates page (grid layout), but the Pages page (grid layout) is unaffected. It seems to be caused by |
|
The current approach looks fine to me, but we might want to define it as a variable. |
ntsekouras
force-pushed
the
fix/patterns-pagination-z-index
branch
from
4e3b9cf
to
13f0953
Compare
|
Thank you for helping me identify the stacking context! I've updated the code to use vars. |
ntsekouras
changed the title
z-indexz-index
|
Flaky tests detected in 13f0953. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7887524818
|
|
Thanks for the update! I think it's okay to merge as is, but what do you think about unifying the variables to If we do that, I think we can delete this class name. |
I'm not sure we should add the I have no strong opinions on this though.. |
t-hamano
approved these changes
Feb 13, 2024
There was a problem hiding this comment.
I'm not sure we should add the
z-index:2to the DataViews package as is specific to some of our views and also contains a small value(2). My thinking is that we should probably not need it and if some consumer does, set the value they have to..
Indeed. I don't have a strong opinion either, so let's ship this 👍
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
…ndex` (#58965) Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: miksansegundo <miguelsansegundo@git.wordpress.org>
|
I just cherry-picked this PR to the more/backports-for-beta3 branch to get it included in the next release: 16f7948 |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
added
the
Backported to WP Core
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
…ndex` (#58965) Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: miksansegundo <miguelsansegundo@git.wordpress.org>
oandregal
reviewed
Apr 1, 2024
| @@ -101,6 +101,10 @@ | |||
| text-overflow: ellipsis; | |||
| color: inherit; | |||
| } | |||
|
|
|||
| .dataviews-pagination { | |||
There was a problem hiding this comment.
@ntsekouras @t-hamano I've noticed that we use a dataviews internal class to do this. Can we do this differently? In my view, consumers shouldn't target dataviews internal classes, otherwise they'll become public API.
There was a problem hiding this comment.
I think the Data Views pagination component cannot be wrapped in an element with a specific class, nor can we add a specific class name.
gutenberg/packages/dataviews/src/pagination.js
Lines 24 to 29 in fc78ac9
For example, should we add a prop like paginationClassName to the DataViews component?
gutenberg/packages/dataviews/src/dataviews.js
Lines 31 to 46 in fc78ac9
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #58929
From the issue:
This is an issue for template and template parts list too, and this PR fixes that too.
Testing Instructions